-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cc-wrapper: fix -mtune= validation, add ARM, add fallbacks #205091
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
mentioned this pull request
Dec 8, 2022
13 tasks
ofborg
bot
added
10.rebuild-darwin: 0
This PR does not cause any packages to rebuild on Darwin
10.rebuild-linux: 0
This PR does not cause any packages to rebuild on Linux
labels
Dec 8, 2022
ghost
marked this pull request as ready for review
December 9, 2022 04:38
Atemu
reviewed
Dec 9, 2022
ghost
marked this pull request as draft
April 24, 2023 02:22
ghost
marked this pull request as ready for review
June 21, 2023 05:14
ghost
requested a review
from Atemu
October 22, 2023 07:24
Atemu
approved these changes
Oct 22, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me.
Before this commit, cc-wrapper/default.nix was using `isGccArchSupported` to validate `-mtune=` values. This has two problems: - On x86, `-mtune=` can take the same values as `-march`, plus two additional values `generic` and `intel` which are not valid for `-march`. - On ARM, `-mtune=` does not take the same values as `-march=`; instead it takes the same values as `-mcpu`. This commit fixes these two problems by adding a new `isGccTuneSupported` function. For `isx86` this returns `true` for the two special values and otherwise defers to `isGccArchSupported`. This commit also adds support for `-mtune=` on Aarch64. Unfortunately on Aarch64, Clang does not accept as wide a variety of `-mtune=` values as Gcc does. In particular, Clang does not tune for big.LITTLE mixed-model chips like the very popular RK3399, which is targeted using `-march=cortex-a72.cortex-a53` in gcc. To address this problem, this commit also adds a function `findBestTuneApproximation` which can be used to map clang-unsupported tunings like `cortex-a72.cortex-a53` to less-precise tunings like `cortex-a53`. The work which led to this commit arose because we now have packages, like `crosvm`, which use *both* `clang` *and* `gcc`. Previously I had been using `overrideAttrs` to set `NIX_CFLAGS_COMPILE` on a package-by-package basis based on which compiler that package used. Since we now have packages which use *both* compilers, this strategy no longer works. I briefly considered splitting `NIX_CFLAGS_COMPILE` into `NIX_CFLAGS_COMPILE_GCC` and `NIX_CFLAGS_COMPILE_CLANG`, but since `NIX_CFLAGS_COMPILE` is sort of a hack to begin with I figured that adding the logic to `cc-wrapper` would be preferable.
Atemu
approved these changes
Oct 22, 2023
Atemu
added
the
12.approvals: 1
This PR was reviewed and approved by one reputable person
label
Oct 22, 2023
ghost
deleted the
clang-tune
branch
October 23, 2023 01:31
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
10.rebuild-darwin: 0
This PR does not cause any packages to rebuild on Darwin
10.rebuild-linux: 0
This PR does not cause any packages to rebuild on Linux
12.approvals: 1
This PR was reviewed and approved by one reputable person
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Before this commit, cc-wrapper/default.nix was using
isGccArchSupported
to validate-mtune=
values. This has three problems:On x86,
-mtune=
can take the same values as-march
, plus two additional valuesgeneric
andintel
which are not valid for-march
.On Arm64,
-mtune=
does not take the same values as-march=
; instead it takes the same values as-mcpu
.On PowerPC, all
-march=
flags are rejected, soisGccArchSupported
ought to unconditionally return false (see cc-wrapper: -march= is not allowed on powerpc #205242), yet-mtune
is allowed.This commit fixes these three problems by adding a new
isGccTuneSupported
function. Forisx86
this returnstrue
for the two special values and otherwise defers toisGccArchSupported
. This commit also adds support for-mtune=
on Arm64, PowerPC, and MIPS.On Arm64, Clang does not accept as wide a variety of
-mtune=
values as Gcc does. In particular, Clang does not tune for big.LITTLE mixed-model chips like the very popular RK3399 (found in 6 of the 33 ARM boards on the NixOS wiki), which is-mtune=cortex-a72.cortex-a53
. To address this problem, this commit also adds a functionfindBestTuneApproximation
which can be used to map clang-unsupported tunings likecortex-a72.cortex-a53
to less-precise tunings likecortex-a53
.The work which led to this commit arose because we now have packages, like
crosvm
, which use bothclang
andgcc
;NIX_CFLAGS_COMPILE
can no longer be used for those packages since it doesn't distinguish between clang and gcc.Things done
gcc.arch="bdver1"; gcc.tune="native"
)gcc.tune="cortex-a72.cortex-a53"
)gcc.tune="power9"
)gcc.tune="octeon3"
)